Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| ], | ||
| }) | ||
| export class SelectComponent<T = unknown> implements AfterContentChecked, AfterViewChecked, ControlValueAccessor { | ||
| inputId = input.required<string>(); |
There was a problem hiding this comment.
add descriptions like in other components
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| > | ||
| @if (searchable()) { | ||
| <div class="tedi-select__search-wrapper"> | ||
| @if (!searchTerm() && selectedValues().length && !multiple()) { |
There was a problem hiding this comment.
Maybe extract this to a computed?
| </span> | ||
| } | ||
| @if (multiple() && selectedValues().length) { | ||
| <div |
There was a problem hiding this comment.
Line 53-105 seems to be exactly the same as line 110-140, maybe let's extract this part to a ng-template?
There was a problem hiding this comment.
This template is quite large and nested which makes it harder to read and maintain. Maybe extract it into smaller components?
There was a problem hiding this comment.
seems like good idea, but might actually add complexity, as elements are quite tightly coupled (state/signals, KB navigation, angular CDK)
| @@ -0,0 +1,283 @@ | |||
| @let listboxId = inputId() + "-listbox"; | |||
There was a problem hiding this comment.
I noticed that Select is the only component where we compute values with @let in the template. For consistency, maybe it would make sense to keep them in the ts file instead?
| * Whether to show a clear button when a value is selected. | ||
| * @default true | ||
| */ | ||
| clearable = input<boolean>(true); |
There was a problem hiding this comment.
For TextField component Kärolin said that this should be false by default.
| * Whether to show a "Select All" option in multiselect mode. | ||
| * @default false | ||
| */ | ||
| selectAll = input<boolean>(false); |
There was a problem hiding this comment.
I would rename this to showSelectAll or something similar.
|
|
||
| &__group-name { | ||
| display: block; | ||
| padding: var(--dropdown-group-label-padding-y, 8px) var(--dropdown-group-label-padding-x, 12px) var(--layout-grid-gutters-04, 4px); |
There was a problem hiding this comment.
Remove px values.
| @@ -0,0 +1,129 @@ | |||
| tedi-dropdown-item-value, | |||
| .tedi-dropdown-item-value { | |||
There was a problem hiding this comment.
Do we need both selectors?
| tedi-dropdown-item-value, | ||
| .tedi-dropdown-item-value { | ||
| display: flex; | ||
| gap: var(--dropdown-item-inner-spacing, 8px); |
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
3 participants
No description provided.